Add the lexicon provider plumbing behind the chooser - #308
Conversation
Introduce LexiconProvider on the port, scope the lexicon registry to the project in view, record the project-to-lexicon link in two project settings, and register FieldWorks Lite as the first provider behind it. Availability and connection are kept apart: software that can be reached but holds no lexicon for this project still answers for its authority, so the refs it minted read as misses rather than as foreign. No chooser and no first-open offer; both stay open on the issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a FieldWorks Lite lexicon provider, project-specific registry wiring, supporting type declarations, extensive tests, PAPI mocks, and mock lookup helpers. It also namespaces tokenization failure logs with the ChangesProject lexicon integration
Interlinearizer logging update
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
The Lexicon extension owns which FW Lite lexicon a Paratext project uses, in its own lexicon.lexiconCode project setting. Read that instead of keeping a second copy: both extensions then reach one lexicon per project, a lexicon chosen in either is the lexicon both use, and clearing it unlinks both. Where a link is recorded becomes the provider's business, the same way reaching the lexicon already was. LexiconProvider gains subscribeToLink, so a provider whose lexicons live elsewhere records its link elsewhere, and interlinearizer.lexiconAuthority and interlinearizer.lexiconCode go away along with the half-a-link state they could represent. Every provider is watched through one effect rather than one hook each, so the hooks useLexiconRegistry runs do not vary with how many providers there are or which of them can be reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Lexicon extension's IEntryService now answers undefined for a lexicon that is not there and rejects only for a fault, so a ref naming a lexicon since deleted misses rather than throwing out of resolveSense. Restate that here, since these declarations track that extension's and are changed with them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… for Three fixes from review, each with a test that fails without it. The registry paired the project in view with links read for the project before it, for the render a switch produces. An effect cleared them, but an effect runs after that render is on screen and after the effects of anything rendered beneath it, so a consumer could search or create in the lexicon of the project it had just left. Links now carry the project they were read for and are dropped on the way out, which also retires the extra render the reset cost. searchByForm kept every entry holding a form in the requested writing system, including entries the lexicon matched on another language's form or on a gloss. The form in the requested writing system now has to be the one that matches, folded on both sides so a match made on a pointed form survives. Availability is gathered with allSettled: a provider that rejects rather than answering false is misbehaving, and one of those left every other provider unreachable. Also give addEntry the @throws its rejection needs, per the repo's comment rules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four fixes from review, each with a test that fails without it. Links were kept against the project they were read for, which a second visit to that project matched. Leaving a project closes its watches, so its link can change unobserved; a reopened project exposed what its last visit saw until its new watch answered. Links are now dropped on the way out of a project instead, which also retires the two guards that were compensating for state outliving its project. The cached entry service was held for the session. The platform revokes a network object's proxy when it is disposed, so every lexicon read would have thrown rather than missed once the Lexicon extension replaced its service. Drop the reference in an onDidDispose handler, without awaiting anything, since the proxy dies as that handler returns. A limit below zero trimmed candidates off the end rather than capping them, so a caller asking for at most -1 got all but one. A watch that fails to close now says so, rather than leaving an unobserved rejection to surface somewhere else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry service declares a sense's gloss and an entry's senses and lexeme form required, but its records arrive from another extension over PAPI, which enforces nothing. Three reads threw a TypeError on a record missing one, and a fourth passed undefined off as a MultiString - a gloss rendering as nothing with no clue why. Every read now defends itself, and each missing field lands on behavior the render ladder already defines: a sense with no gloss is unglossed, an entry with no senses offers nothing to link to, an entry with no lexeme form matches no writing system. Creating an entry that comes back senseless now reaches the error that says so rather than throwing on the way to it - the optional chain there guarded the entry but not its senses. Defaulting rather than dropping records: every outcome dropping would produce is the one defaulting already gives, without a rule per field to maintain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the text that justified this change rather than describing the code: the closed "Current Lexicon gap" notes, the reviewer-facing aside on declaring the setting key twice, and the session-versus-project contrast in the hook. Fix two rule violations: `subscribeToLink` documented one of its two parameters, as did `getMockedPapiFn`. Fold both notes into the summary and drop the `@returns` that restated `UnsubscriberAsync`. Correct two comments that had gone wrong: the one on `disposed` said it guards state updates alone and then credited it with unsubscribing too, and `createLexiconRegistry` still spoke of lexicons connected for the session after the registry became per-project. State the guarantee rather than the callee in the refs and in `matchesInWritingSystem`, drop the fourfold repetition of "leaving a project drops its links", and split the longest sentences. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
69ce5a9 to
840aa5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 15 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on imnasnainaec).
src/utils/fw-lite-lexicon.ts line 47 at r2 (raw file):
*/ async function getEntryService(): Promise<LexiconEntryService | undefined> { if (entryService) return entryService;
getEntryService caches the resolved value rather than the in-flight promise, so the cache is only set after both awaits complete. Concurrent callers therefore each open their own waitForNetworkObject and their own networkObjects.get. I probed this: two concurrent isAvailable() calls produce 2 waits and 2 fetches, and so do two concurrent resolveSense calls.
Nothing reads wrong as a result — the duplicate onDidDispose handlers this leaves on one service are idempotent. The cost is the wait. When FW Lite is absent, every concurrent batch of lexicon operations blocks on its own 10s timeout instead of sharing one, which is the shape of a first paint that resolves several refs at once.
The doc above the cache says a session "pays the wait once rather than once per connection", which is true serially and not concurrently — so this reads as the code missing its stated intent rather than as a deliberate simplification.
Caching the promise instead of the value closes it. The platform ships cached-initializer for exactly this (papi.d.ts:5023), and its doc notes it clears the cached promise on failure, which preserves the retry-after-a-failed-lookup behavior this has today and is worth keeping.
Drafted by Claude Opus 5
src/utils/fw-lite-lexicon.ts line 56 at r2 (raw file):
// The proxy is revoked as soon as these handlers return, so this one only drops the reference: // anything it awaited first would be acting on a dead proxy. service?.onDidDispose(() => {
This handler closes over nothing that identifies which service it belongs to, so it clears whatever happens to be cached at the moment it fires rather than the service it was registered for. If the platform fires a disposed object's handlers after a replacement has been cached, the live service is dropped and the next call re-pays the wait. Probed and confirmed.
Lower priority than the caching one above, since it needs either a double-fire or a second handler created by that concurrency. Same fix shape though: capture service and clear only when entryService === service.
Drafted by Claude Opus 5
getEntryService cached the resolved service rather than the look-up, so callers arriving while one was in flight each started their own wait and fetch. Hold the promise instead, and clear it once it settles: a found service still answers from the value cache, and a look-up that found none is started afresh rather than pinned as a permanent miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The handler cleared whatever was cached at the moment it fired, so a disposed service firing again after a replacement had been cached would drop the live one and make the next call re-pay the wait. Capture the service and clear only on a match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 2 comments and resolved 2 discussions.
Reviewable status: 13 of 15 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
src/utils/fw-lite-lexicon.ts line 47 at r2 (raw file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
getEntryServicecaches the resolved value rather than the in-flight promise, so the cache is only set after both awaits complete. Concurrent callers therefore each open their ownwaitForNetworkObjectand their ownnetworkObjects.get. I probed this: two concurrentisAvailable()calls produce 2 waits and 2 fetches, and so do two concurrentresolveSensecalls.Nothing reads wrong as a result — the duplicate
onDidDisposehandlers this leaves on one service are idempotent. The cost is the wait. When FW Lite is absent, every concurrent batch of lexicon operations blocks on its own 10s timeout instead of sharing one, which is the shape of a first paint that resolves several refs at once.The doc above the cache says a session "pays the wait once rather than once per connection", which is true serially and not concurrently — so this reads as the code missing its stated intent rather than as a deliberate simplification.
Caching the promise instead of the value closes it. The platform ships
cached-initializerfor exactly this (papi.d.ts:5023), and its doc notes it clears the cached promise on failure, which preserves the retry-after-a-failed-lookup behavior this has today and is worth keeping.Drafted by Claude Opus 5
createCachedInitializer isn't reachable from an extension: it's declared in shared/utils/cached-initializer, which .eslintrc bans under shared/* and webpack doesn't list in externals. Hand-rolled the same shape here. paranext/paranext-core#2845 exports it.
src/utils/fw-lite-lexicon.ts line 56 at r2 (raw file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
This handler closes over nothing that identifies which service it belongs to, so it clears whatever happens to be cached at the moment it fires rather than the service it was registered for. If the platform fires a disposed object's handlers after a replacement has been cached, the live service is dropped and the next call re-pays the wait. Probed and confirmed.
Lower priority than the caching one above, since it needs either a double-fire or a second handler created by that concurrency. Same fix shape though: capture
serviceand clear only whenentryService === service.Drafted by Claude Opus 5
Done.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
The chooser #308's plumbing was built for. A Project-menu item hands off to the lexicon software's own way of choosing or creating a lexicon, and the project is linked to whatever the user settles on. Nothing here names FieldWorks Lite, and nothing here shows the choosing. A second provider adds a chooser of its own rather than a case to this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
The plumbing a lexicon chooser needs, with no chooser: a
LexiconProvideron the port, a registryscoped to the project in view, and FieldWorks Lite registered behind it as the first provider.
Nothing in the registry or the analysis model names FieldWorks Lite. A second provider later means
registering one, not revising this.
Partially addresses #44; the chooser and the first-open offer are still open there.
What this adds
LexiconProvider—{ authority, isAvailable(), subscribeToLink(), connect(lexiconId?) },alongside the existing
LexiconResolver.be live at once.
connect()with no lexicon id is the load-bearing case: the resolver declares the authority andholds nothing, so a project with no link reads a FieldWorks Lite ref as a miss rather than
as foreign. Same rendered output, different fact — the one FW Lite adapter: resolve lexicon refs for display #227's link indicator needs.
useLexiconRegistry(projectId).this project is linked to.
subscribeToLinkreads itslexicon.lexiconCoderatherthan keeping a copy.
lexicon.entryServicenetworkobject, and a resolver over
getSense/getEntries/addEntry.Decisions
lexicon.lexiconCodecannot name a non-FW-Lite lexicon, so one link slot here would need asecond mechanism for Allow user to gloss internally and through Lexicon extension #46's tier-1 lexicon.
that minted them, and an affordance goes to the first available provider that can serve it.
fw-liteauthority, whichever store backs the lexicon.second copy.
id.
survive untouched.
search/createwhen connected,allomorphs/msasnever.speech without the inflection class and stem features an analysis needs.
rule per field to reach the same outcomes.
its link can then change unobserved.
Two things reviewers should weigh
The Lexicon extension's types are restated locally (
src/types/lexicon-extension.ts, plus itssetting key in
src/types/lexicon-extension-settings.d.ts), because its own declarations reach abuild only where that extension is installed — which CI and a fresh clone cannot assume. Its
declarations remain the standard. Publishing them as a package is worth filing, not worth gating
this.
resolveSense/searchByForm/createEntryare implemented here rather than left to #227.What remains of #227 is the display wiring, which is UI and out of scope by the same rule that
leaves out the chooser. Two presentational mappings do go there: a sense's
definition(rich textin FW Lite) and its
senseLabel(FW Lite does not number senses).Deliberately not added
listLexicons(),createLexicon(draft),displayName, and any command that opens the selector.The chooser is #44's, and the choosing affordance belongs to the provider.
Housekeeping
The "Current Lexicon gap" notes on
EntryRefandSenseRefare updated:getEntryandgetSensehave landed. The allomorph and MSA gaps stand.
Test plan
npm run lint(ESLint + stylelint +tsc --noEmit+ dependency scope) — clean.npm run test:coverage— 2441 tests pass, the 100% threshold holds, and every touched or addedsource file is at 100%.
fw-lite-lexicon.test.tscovers availability (never registered, unfetchable, disposed andreplaced), the unconnected resolver, foreign and unlabelled refs, writing-system narrowing,
limits, entry creation, records missing a declared field, and the link watch.
useLexiconRegistrycovers the first-render shape, relink and clear while open, projectswitches and reopens, teardown races, and a provider that rejects.
extension for the project in view is the way to try it.
Depends on
sillsdev/languageforge-lexbox#2627 — this consumes
IEntryServicein its lexicon-addressed form andreads that extension's project setting.
Merge that first.Merged.This change is
Summary by CodeRabbit
New Features
Bug Fixes